CNTRLPLANE-3851: Oauth server proxy config e2e - #31463
Conversation
…sions Add helper functions for deploying a Squid forward proxy, managing proxy-scoped network policies, configuring component-scoped proxy on the Authentication CR, and verifying OAuth server deployment alignment. Extend the keycloak client with group/audience mapper creation, client configuration, and client lookup by clientID. Add crypto helpers for generating self-signed CA and server certificates used by the Squid proxy's HTTPS listener. Refactor keycloak deployment cleanup to rely on namespace cascading, reducing cleanup API calls from 6 to 2 (namespace + CA configmap in openshift-config). All deploy helpers self-clean on error and return nil cleanups, preventing resource leaks when callers use Expect before DeferCleanup.
Use BeforeEach to deploy Squid proxy, Keycloak, and save/restore the proxy config, with DeferCleanup for teardown. Each test only handles its own IdP registration, network policies, and verification. Three tests validate: - OIDC IdP discovery through an HTTP proxy - OIDC IdP discovery through an HTTPS proxy with trustedCA - Fallback on spec.proxy removal (deletes the proxy to prove the operator no longer routes through it)
- Use Keycloak service URL as OIDC issuer instead of route URL so the network policy structurally enforces proxy usage (only the proxy namespace can reach Keycloak pods directly). - Remove ingress router rule from network policy since the service URL bypasses the router. - Use service CA for IdP CA configmap instead of ingress CA, matching the service serving cert signer. - Replace saveAndRestoreProxyConfig and saveAndRestoreIdPs with a single saveAndRestoreAuthState that snapshots both the Authentication operator CR and oauth/cluster, restoring both on cleanup and waiting for stabilization only if changes were made. - Remove ListClientsRaw in favor of typed ListClients with RedirectURIs field; fix clientId JSON tag; simplify UpdateClientRaw to flat merge. - Add must prefix to crypto helpers to make panic-on-error explicit. - Remove dead code: idpCleanupWrapper, cleanIdentityProviderByName, resetComponentProxyState.
Deploy helpers no longer self-clean on error. Instead they always return accumulated cleanups, and callers register DeferCleanup before calling Expect. This prevents resource leaks when BeforeEach aborts mid-setup.
Fixes nosprintfhostport linter warning.
Replace waitForClusterOperatorAvailableNotProgressingNotDegraded and its supporting functions with the existing operator.WaitForOperatorsToSettle utility. This checks all operators for the same three conditions (Available, NotProgressing, NotDegraded), which is appropriate for serial conformance tests.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@ehearne-redhat: This pull request references CNTRLPLANE-3851 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ehearne-redhat The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds an end-to-end authentication component proxy suite and helpers. The tests deploy Squid and Keycloak, configure proxies, trusted CAs, and ChangesAuthentication component proxy behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AuthenticationTest
participant OAuthServer
participant Squid
participant Keycloak
AuthenticationTest->>OAuthServer: start OIDC login
OAuthServer->>Squid: send proxied OIDC request
Squid->>Keycloak: forward OIDC request
Keycloak-->>OAuthServer: return OIDC response
OAuthServer-->>AuthenticationTest: complete login
AuthenticationTest->>OAuthServer: remove proxy configuration
OAuthServer->>Keycloak: send direct OIDC request
Keycloak-->>OAuthServer: return direct response
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
test/extended/authentication/keycloak_helpers.go (1)
46-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the per-resource cleanups are discarded.
The service account, service, deployment, and route cleanups are dropped because the namespace cleanup removes those objects. The CA ConfigMap cleanup stays because that object lives in
openshift-config. Add one short comment so a later reader does not treat the discarded returns as a leak.As per coding guidelines: "Keep comments minimal and helpful, explaining why rather than what."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/keycloak_helpers.go` around lines 46 - 79, Add one concise comment near the cleanup initialization or before the per-resource creation calls explaining that the service account, service, deployment, and route cleanups are intentionally discarded because namespace cleanup removes those objects, while the CA ConfigMap cleanup is retained because it lives in openshift-config. Do not alter the cleanup behavior.Source: Coding guidelines
test/extended/authentication/component_proxy_helpers.go (3)
406-424: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClient selection is nondeterministic.
The master realm contains several clients with
redirectUris, for exampleaccount,account-console, andsecurity-admin-console. The loop takes the first one the API returns, so the test can configure the IdP with an unintended client. Select the client by its knownclientIDinstead.♻️ Suggested approach
- var adminClientID, passwdClientID string - for _, c := range clientList { - if c.ClientID == "admin-cli" { - adminClientID = c.ID - } else if len(c.RedirectURIs) > 0 { - passwdClientID = c.ID - setup.clientID = c.ClientID - } - if len(passwdClientID) > 0 && len(adminClientID) > 0 { - break - } - } + var adminClientID, passwdClientID string + for _, c := range clientList { + switch c.ClientID { + case "admin-cli": + adminClientID = c.ID + case keycloakTestClientID: // the client created for this test + passwdClientID = c.ID + setup.clientID = c.ClientID + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_helpers.go` around lines 406 - 424, Update the client-selection loop to choose the password-grant client by its known clientID rather than selecting the first client with non-empty RedirectURIs. Preserve admin-cli lookup and the existing missing-client errors, and assign setup.clientID from the explicitly matched password-grant client.
333-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTraffic detection reads the full log history.
getSquidProxyLogspasses a zero time, so the check matches any earlier request as well.TCP_also appears in unrelated log lines. If a spec must prove that a specific step produced proxy traffic, pass a start timestamp togetSquidProxyLogsSince.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_helpers.go` around lines 333 - 347, Update waitForSquidProxyTraffic to capture the check start timestamp and use getSquidProxyLogsSince with that timestamp on each poll, limiting detection to traffic generated after the wait began. Retain the existing polling and error behavior while using a more specific proxy request pattern than the broad TCP_ match.
689-692: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe debug message can report the wrong actual state.
matchTrustedCAVolumechecks both the volume and the mount. Afalseresult does not prove that presence equals!expectTrustedCAVolume. Log the two found flags instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_helpers.go` around lines 689 - 692, Update the logging in the matchTrustedCAVolume failure branch to report the actual trusted CA volume and mount presence flags returned or computed by that check, rather than deriving a single state from !expectTrustedCAVolume. Keep the existing mismatch return behavior unchanged.test/extended/authentication/crypto_helpers.go (1)
18-89: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider ECDSA P-256 and shared key/serial generation.
The path instructions prefer Ed25519 or ECDSA P-256+ for signing. These certificates are test-only and short-lived, so RSA-2048 with SHA-256 is acceptable, but ECDSA P-256 generates faster and matches the guidance. The two functions also duplicate key generation and serial-number generation; extract a small helper.
As per path instructions: "Signing: Ed25519 or ECDSA P-256+".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/crypto_helpers.go` around lines 18 - 89, Update mustNewServerCertificate and mustNewCertificateAuthority to use ECDSA P-256 keys and the corresponding certificate signature algorithm instead of RSA-2048/SHA-256. Extract the duplicated private-key and serial-number generation into a small shared helper, then reuse it in both certificate-construction paths while preserving their existing certificate hierarchy and fields.Source: Path instructions
test/extended/authentication/component_proxy_oauth.go (3)
363-373: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the "no redeploy" assertion.
The pod-name comparison passes immediately after the CA rotation, before the operator could have rolled out a new revision. Compare pod UIDs, or assert that the
oauth-openshiftDeploymentmetadata.generationandstatus.observedGenerationdid not change, and hold the assertion witho.Consistentlyfor a short window. That distinguishes "not redeployed" from "not yet redeployed".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_oauth.go` around lines 363 - 373, Strengthen the no-redeploy check in the pod verification block by polling consistently for a short window instead of comparing names only once. Capture and compare stable pod UIDs (or the oauth-openshift Deployment generation and observedGeneration), and ensure the assertion remains unchanged throughout the window so delayed rollouts are detected.
101-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
updateAuthenticationProxyinstead of inlining Get/Update.
component_proxy.gosets the component proxy throughupdateAuthenticationProxy. This file repeats the Get/mutate/Update sequence in four specs (lines 101-110, 146-159, 174-182, 258-268, 378-391). The helper also centralizes conflict handling if it is added later. Use the helper here for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_oauth.go` around lines 101 - 110, Replace the inline Authentication Get, proxy mutation, and Update sequence in the affected specs with the existing updateAuthenticationProxy helper, passing the appropriate proxy configuration and preserving each test’s existing assertions and behavior. Apply this consistently to all repeated occurrences in the file.
204-209: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleeps before positive log assertions with
o.Eventually.Lines 205, 231, 289, 358, and 406 each sleep two minutes unconditionally. That adds about ten minutes to the suite. For the positive assertions (lines 209, 292, 361), poll
getSquidProxyLogsSincewitho.Eventuallyso the spec continues as soon as the expected log line appears. For the negative assertions (lines 235, 410), a bounded wait is still needed;o.Consistentlyexpresses that intent more clearly than a sleep.Also note that lines 193 and 225 use
logCutOffandlogCutofffor the same concept. Use one spelling.♻️ Proposed change for the positive assertion
- g.By("Waiting for squid logs to settle before checking for proxy traffic") - time.Sleep(2 * time.Minute) - - logs, err := getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff) - o.Expect(err).NotTo(o.HaveOccurred()) - o.Expect(logs).To(o.ContainSubstring(keycloakHost), "squid logs should contain keycloak traffic after proxy login") + g.By("Waiting for squid logs to show proxy traffic") + o.Eventually(func() (string, error) { + return getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff) + }).WithTimeout(3*time.Minute).WithPolling(10*time.Second). + Should(o.ContainSubstring(keycloakHost), "squid logs should contain keycloak traffic after proxy login")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_oauth.go` around lines 204 - 209, Replace the unconditional time.Sleep call before the positive assertion with o.Eventually that polls getSquidProxyLogsSince until the expected substring appears in the logs. Apply this pattern to all lines with positive assertions (checking that logs contain keycloakHost or similar expected values) by moving the getSquidProxyLogsSince call and the o.ContainSubstring check into the Eventually block. For negative assertions (checking that logs do not contain something), use o.Consistently instead to express the intent of verifying absence over time. Additionally, standardize the spelling throughout the file to use logCutOff consistently instead of mixing logCutOff and logCutoff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Line 194: The Replicas field assignment uses the new builtin incorrectly by
passing a value instead of a type. Add the import for "k8s.io/utils/ptr" and
replace new(int32(1)) with ptr.To(int32(1)), following the same pattern already
used in keycloak_helpers.go.
- Around line 278-284: Update the watch.Error branch in the event callback to
stop passing the runtime.Object event.Object to the %w formatting verb; use a
non-error formatting verb such as %v, or explicitly convert the object to an
error before wrapping. Preserve the existing error message context and return
behavior, then ensure go vet passes.
In `@test/extended/authentication/component_proxy_oauth.go`:
- Around line 60-69: Remove the local kcCleanups declaration in the Keycloak
setup so the assignment updates the suite-level variable used by the deferred
cleanup closure. Reset the suite-level kcCleanups slice in BeforeEach before
deploying resources, ensuring later IdP cleanup functions appended in each spec
are executed without accumulating across specs.
- Around line 405-410: Move the existing logCutOff declaration in the noProxy
test to immediately before the login call, then replace getSquidProxyLogs with
getSquidProxyLogsSince using that cutoff for the negative assertion. Preserve
the existing error and absence checks while limiting logs to entries generated
after the cutoff.
In `@test/extended/authentication/component_proxy.go`:
- Around line 34-43: Register each cleanup only after its helper error assertion
succeeds: in test/extended/authentication/component_proxy.go lines 34-43, move
the assertions before DeferCleanup(authRestore) and DeferCleanup(proxyCleanup);
in test/extended/authentication/component_proxy_oauth.go lines 88-91, move the
assertion before DeferCleanup(authRestore).
- Around line 34-61: The cleanup registrations in the setup block currently
restore authentication state after deleting Squid and Keycloak resources. Move
the g.DeferCleanup(authRestore) registration to after the proxy and Keycloak
cleanup registrations, matching the ordering used by component_proxy_oauth.go so
authentication is restored first during LIFO teardown.
In `@test/extended/authentication/keycloak_client.go`:
- Around line 230-244: Update UpdateClientRaw to merge the nested attributes map
with the existing attributes before issuing the client update, preserving all
unrelated client attributes while applying changes such as
access.token.lifespan. Keep the existing shallow merge behavior for other
top-level fields and ensure the merged attributes are included in the final
update payload.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 406-424: Update the client-selection loop to choose the
password-grant client by its known clientID rather than selecting the first
client with non-empty RedirectURIs. Preserve admin-cli lookup and the existing
missing-client errors, and assign setup.clientID from the explicitly matched
password-grant client.
- Around line 333-347: Update waitForSquidProxyTraffic to capture the check
start timestamp and use getSquidProxyLogsSince with that timestamp on each poll,
limiting detection to traffic generated after the wait began. Retain the
existing polling and error behavior while using a more specific proxy request
pattern than the broad TCP_ match.
- Around line 689-692: Update the logging in the matchTrustedCAVolume failure
branch to report the actual trusted CA volume and mount presence flags returned
or computed by that check, rather than deriving a single state from
!expectTrustedCAVolume. Keep the existing mismatch return behavior unchanged.
In `@test/extended/authentication/component_proxy_oauth.go`:
- Around line 363-373: Strengthen the no-redeploy check in the pod verification
block by polling consistently for a short window instead of comparing names only
once. Capture and compare stable pod UIDs (or the oauth-openshift Deployment
generation and observedGeneration), and ensure the assertion remains unchanged
throughout the window so delayed rollouts are detected.
- Around line 101-110: Replace the inline Authentication Get, proxy mutation,
and Update sequence in the affected specs with the existing
updateAuthenticationProxy helper, passing the appropriate proxy configuration
and preserving each test’s existing assertions and behavior. Apply this
consistently to all repeated occurrences in the file.
- Around line 204-209: Replace the unconditional time.Sleep call before the
positive assertion with o.Eventually that polls getSquidProxyLogsSince until the
expected substring appears in the logs. Apply this pattern to all lines with
positive assertions (checking that logs contain keycloakHost or similar expected
values) by moving the getSquidProxyLogsSince call and the o.ContainSubstring
check into the Eventually block. For negative assertions (checking that logs do
not contain something), use o.Consistently instead to express the intent of
verifying absence over time. Additionally, standardize the spelling throughout
the file to use logCutOff consistently instead of mixing logCutOff and
logCutoff.
In `@test/extended/authentication/crypto_helpers.go`:
- Around line 18-89: Update mustNewServerCertificate and
mustNewCertificateAuthority to use ECDSA P-256 keys and the corresponding
certificate signature algorithm instead of RSA-2048/SHA-256. Extract the
duplicated private-key and serial-number generation into a small shared helper,
then reuse it in both certificate-construction paths while preserving their
existing certificate hierarchy and fields.
In `@test/extended/authentication/keycloak_helpers.go`:
- Around line 46-79: Add one concise comment near the cleanup initialization or
before the per-resource creation calls explaining that the service account,
service, deployment, and route cleanups are intentionally discarded because
namespace cleanup removes those objects, while the CA ConfigMap cleanup is
retained because it lives in openshift-config. Do not alter the cleanup
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 708f097b-0dec-43c5-b832-fe6b994bdc5a
📒 Files selected for processing (7)
test/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/component_proxy_oauth.gotest/extended/authentication/crypto_helpers.gotest/extended/authentication/keycloak_client.gotest/extended/authentication/keycloak_helpers.gotest/extended/authentication/operator_status_helpers.go
| Labels: map[string]string{"app": squidServiceName}, | ||
| }, | ||
| Spec: appsv1.DeploymentSpec{ | ||
| Replicas: new(int32(1)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
new(int32(1)) does not compile.
The new builtin accepts a type, not a value. Use ptr.To as keycloak_helpers.go does at line 185.
🐛 Proposed fix
- Replicas: new(int32(1)),
+ Replicas: ptr.To(int32(1)),Add the import:
"k8s.io/utils/ptr"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Replicas: new(int32(1)), | |
| Replicas: ptr.To(int32(1)), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy_helpers.go` at line 194, The
Replicas field assignment uses the new builtin incorrectly by passing a value
instead of a type. Add the import for "k8s.io/utils/ptr" and replace
new(int32(1)) with ptr.To(int32(1)), following the same pattern already used in
keycloak_helpers.go.
| g.By("Waiting for squid logs to settle before checking for absence of proxy traffic") | ||
| time.Sleep(2 * time.Minute) | ||
|
|
||
| logs, err := getSquidProxyLogs(ctx, oc, proxyNamespace) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use getSquidProxyLogsSince with a cutoff for this negative assertion.
This check reads the full squid log for the pod lifetime. Any keycloak line logged before the noProxy configuration took effect fails the assertion. The other three log checks in this file use a cutoff. Record a cutoff before the login and use getSquidProxyLogsSince.
🐛 Proposed fix
+ logCutOff := time.Now()
+
g.By("Verifying OIDC login works after setting proxy with noProxy")
assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup)
g.By("Waiting for squid logs to settle before checking for absence of proxy traffic")
time.Sleep(2 * time.Minute)
- logs, err := getSquidProxyLogs(ctx, oc, proxyNamespace)
+ logs, err := getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff)
o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect")Move the logCutOff declaration above the login call at line 403.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| g.By("Waiting for squid logs to settle before checking for absence of proxy traffic") | |
| time.Sleep(2 * time.Minute) | |
| logs, err := getSquidProxyLogs(ctx, oc, proxyNamespace) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect") | |
| logCutOff := time.Now() | |
| g.By("Verifying OIDC login works after setting proxy with noProxy") | |
| assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup) | |
| g.By("Waiting for squid logs to settle before checking for absence of proxy traffic") | |
| time.Sleep(2 * time.Minute) | |
| logs, err := getSquidProxyLogsSince(ctx, oc, proxyNamespace, logCutOff) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(logs).NotTo(o.ContainSubstring(keycloakHost), "squid logs should not contain keycloak connect") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy_oauth.go` around lines 405 -
410, Move the existing logCutOff declaration in the noProxy test to immediately
before the login call, then replace getSquidProxyLogs with
getSquidProxyLogsSince using that cutoff for the negative assertion. Preserve
the existing error and absence checks while limiting logs to entries generated
after the cutoff.
| g.By("Saving auth state for restore after test") | ||
| authRestore, err := saveAndRestoreAuthState(ctx, oc) | ||
| g.DeferCleanup(authRestore) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| g.By("Deploying Squid forward proxy") | ||
| var proxyCleanup removalFunc | ||
| httpProxyURL, httpsProxyURL, caCertPEM, proxyNamespace, proxyCleanup, err = deploySquidProxy(ctx, oc) | ||
| g.DeferCleanup(proxyCleanup) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Register cleanup functions only after you assert the error. Both files pass a cleanup function to g.DeferCleanup before checking the error from the helper that returned it. If the helper fails and returns a nil function, g.DeferCleanup receives nil and panics, which replaces the real assertion failure.
test/extended/authentication/component_proxy.go#L34-L43: moveo.Expect(err).NotTo(o.HaveOccurred())aboveg.DeferCleanup(authRestore)and aboveg.DeferCleanup(proxyCleanup).test/extended/authentication/component_proxy_oauth.go#L88-L91: moveo.Expect(err).NotTo(o.HaveOccurred())aboveg.DeferCleanup(authRestore).
📍 Affects 2 files
test/extended/authentication/component_proxy.go#L34-L43(this comment)test/extended/authentication/component_proxy_oauth.go#L88-L91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy.go` around lines 34 - 43,
Register each cleanup only after its helper error assertion succeeds: in
test/extended/authentication/component_proxy.go lines 34-43, move the assertions
before DeferCleanup(authRestore) and DeferCleanup(proxyCleanup); in
test/extended/authentication/component_proxy_oauth.go lines 88-91, move the
assertion before DeferCleanup(authRestore).
| func (kc *keycloakClient) UpdateClientAccessTokenTimeout(id string, timeout int32) error { | ||
| return kc.UpdateClientRaw(id, map[string]any{ | ||
| "attributes": map[string]any{ | ||
| "access.token.lifespan": strconv.FormatInt(int64(timeout), 10), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error { | ||
| existing, err := kc.GetClientRaw(id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| maps.Copy(existing, changes) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
maps.Copy drops existing nested attributes.
maps.Copy performs a shallow merge. When UpdateClientAccessTokenTimeout passes an attributes map with one key, that map replaces the client's whole existing attributes object in the PUT body. Keycloak then loses every other attribute on the client, for example post.logout.redirect.uris. Merge nested maps one level deep, or set only the single attribute key.
🐛 Proposed fix
func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error {
existing, err := kc.GetClientRaw(id)
if err != nil {
return err
}
- maps.Copy(existing, changes)
+ // Merge one level deep so callers that set a single nested key (for example
+ // "attributes") do not discard the client's other nested values.
+ for key, value := range changes {
+ newNested, newIsMap := value.(map[string]any)
+ oldNested, oldIsMap := existing[key].(map[string]any)
+ if newIsMap && oldIsMap {
+ maps.Copy(oldNested, newNested)
+ continue
+ }
+ existing[key] = value
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (kc *keycloakClient) UpdateClientAccessTokenTimeout(id string, timeout int32) error { | |
| return kc.UpdateClientRaw(id, map[string]any{ | |
| "attributes": map[string]any{ | |
| "access.token.lifespan": strconv.FormatInt(int64(timeout), 10), | |
| }, | |
| }) | |
| } | |
| func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error { | |
| existing, err := kc.GetClientRaw(id) | |
| if err != nil { | |
| return err | |
| } | |
| maps.Copy(existing, changes) | |
| func (kc *keycloakClient) UpdateClientAccessTokenTimeout(id string, timeout int32) error { | |
| return kc.UpdateClientRaw(id, map[string]any{ | |
| "attributes": map[string]any{ | |
| "access.token.lifespan": strconv.FormatInt(int64(timeout), 10), | |
| }, | |
| }) | |
| } | |
| func (kc *keycloakClient) UpdateClientRaw(id string, changes map[string]any) error { | |
| existing, err := kc.GetClientRaw(id) | |
| if err != nil { | |
| return err | |
| } | |
| // Merge one level deep so callers that set a single nested key (for example | |
| // "attributes") do not discard the client's other nested values. | |
| for key, value := range changes { | |
| newNested, newIsMap := value.(map[string]any) | |
| oldNested, oldIsMap := existing[key].(map[string]any) | |
| if newIsMap && oldIsMap { | |
| maps.Copy(oldNested, newNested) | |
| continue | |
| } | |
| existing[key] = value | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/keycloak_client.go` around lines 230 - 244,
Update UpdateClientRaw to merge the nested attributes map with the existing
attributes before issuing the client update, preserving all unrelated client
attributes while applying changes such as access.token.lifespan. Keep the
existing shallow merge behavior for other top-level fields and ensure the merged
attributes are included in the final update payload.
Replace scattered DeferCleanup calls in BeforeEach with a shared cleanups slice and explicit AfterEach. Drop the network policy cleanup since the keycloak namespace deletion cascades to it.
Replace custom crypto_helpers.go with library-go's MakeSelfSignedCAConfigForDuration and CA.MakeServerCert. This reuses existing, well-tested crypto utilities instead of maintaining a separate implementation.
The network policy structurally enforces proxy usage — only the proxy namespace can reach Keycloak pods. If the operator successfully discovers the OIDC issuer, it must have gone through the proxy. The log check was redundant.
The service URL approach doesn't work because .svc is in the operator's NO_PROXY list, bypassing the proxy entirely. Revert to the route URL as the OIDC issuer and use haproxy.router.openshift.io/ip_whitelist on the Keycloak route to restrict access to only the Squid proxy pod IP. This structurally enforces proxy usage: the ingress router rejects requests from any IP other than the proxy, so the operator must route through the proxy to reach Keycloak. Also reverts to the default ingress CA (instead of service CA) since the route's TLS cert is signed by the ingress CA.
Replace network policy with haproxy.router.openshift.io/ip_whitelist on the Keycloak route, restricting access to the Squid proxy pod IP. This structurally enforces proxy usage since the ingress router rejects requests from any other source IP. Add a reachability check that verifies Keycloak is blocked without the proxy (expects EOF from the whitelisted route). Wait for trustedCA sync before registering the IdP to avoid a race where the operator tries to use the HTTPS proxy before the CA is available. Revert to route URL as issuer and ingress CA for the IdP CA configmap, since .svc addresses are in the operator's NO_PROXY list.
c2dc765 to
1a6a523
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/authentication/component_proxy.go`:
- Around line 62-67: The removeResources function call in the AfterEach block
discards its error return value using the blank identifier, which violates Go
error handling practices and could hide cleanup failures. Capture the error
return from removeResources instead of discarding it, then assert that the error
did not occur using the same expectation pattern applied to the
WaitForOperatorsToSettle error below it, ensuring cleanup failures are properly
reported before the test completes.
- Around line 32-34: Update the test setup and cleanup flow around BeforeEach
and AfterEach to create a 30-second timeout context with cancellation for
cleanup, pass that cleanup context to removeResources instead of the root ctx,
and handle the returned error explicitly rather than ignoring it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cfd3ac2e-3f0c-41ca-8904-931be371befa
📒 Files selected for processing (3)
test/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/component_proxy_oauth.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended/authentication/component_proxy_oauth.go
- test/extended/authentication/component_proxy_helpers.go
| g.BeforeEach(func() { | ||
| ctx = context.Background() | ||
| cleanups = nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
cat -n test/extended/authentication/component_proxy.go | head -80Repository: openshift/origin
Length of output: 3378
🏁 Script executed:
# Search for the removeResources function to understand error handling
rg -A 10 "func removeResources" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check the full cleanup lifecycle in the file
rg -B 5 -A 5 "cleanups" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 1641
🏁 Script executed:
# Search for removeResources in the entire test/extended directory
rg -l "func removeResources" test/extended/Repository: openshift/origin
Length of output: 191
🏁 Script executed:
# Also check if removeResources is defined in this file
rg "removeResources" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 280
🏁 Script executed:
# Get the full file to understand the structure
wc -l test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 206
🏁 Script executed:
# Get the removeResources function definition from oidc.go
rg -B 3 -A 15 "func removeResources" test/extended/authentication/oidc.goRepository: openshift/origin
Length of output: 837
🏁 Script executed:
# Check the removalFunc type definition
rg "type removalFunc" test/extended/authentication/Repository: openshift/origin
Length of output: 236
🏁 Script executed:
# Check Ginkgo context best practices by looking at other test files
rg "context.Background()" test/extended/ | head -20Repository: openshift/origin
Length of output: 2525
🏁 Script executed:
# Check if there are other patterns for bounded contexts in test files
rg -A 3 "g.BeforeEach" test/extended/authentication/oidc.go | head -30Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check how other test files handle context in AfterEach/cleanup
rg -B 2 -A 5 "g.AfterEach" test/extended/authentication/oidc.goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
# Look for patterns where context is created with timeout/deadline in test files
rg "context.WithTimeout\|context.WithDeadline" test/extended/ | head -20Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check the Ginkgo spec context documentation or common usage
rg "SpecContext\|context.TODO" test/extended/ | head -20Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Verify if there are any timeouts applied in removeResources or similar cleanup operations
rg -B 5 -A 5 "AfterEach" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 629
🏁 Script executed:
# Check if there's a pattern in how Ginkgo tests are structured
# Look for Ginkgo spec that has context with timeout
rg "g.It\|g.Describe" test/extended/authentication/component_proxy.go -A 3 | head -20Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check what the spec timeout is for this test
rg "Timeout\|SetDefaultEventuallyTimeout" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
# Let me check if Ginkgo provides context management
rg "context.WithTimeout" test/extended/ -c | head -5Repository: openshift/origin
Length of output: 387
🏁 Script executed:
# Check the learning context around cleanup patterns
# See if any tests use a special cleanup context
rg -B 3 -A 8 "g.DeferCleanup\|deferred.*cleanup" test/extended/ | head -40Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Check component_proxy_helpers.go for context usage patterns
cat -n test/extended/authentication/component_proxy_helpers.go | head -50Repository: openshift/origin
Length of output: 1905
🏁 Script executed:
# Look for how other Go tests in the codebase handle context in BeforeEach/AfterEach
rg -B 5 -A 10 "g.BeforeEach.*func\(\)" test/extended/storage/driver_configuration.go | head -40Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Verify the exact error handling at line 63
sed -n '62,65p' test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 273
🏁 Script executed:
# Check if context.Background() is the standard pattern in Ginkgo test files
rg "ctx = context.Background()" test/extended/ -cRepository: openshift/origin
Length of output: 370
🏁 Script executed:
# Check the exact error behavior of removeResources - does it always return an error?
sed -n '1,200p' test/extended/authentication/oidc.go | grep -A 20 "func removeResources"Repository: openshift/origin
Length of output: 154
🏁 Script executed:
# Look for patterns in how removeResources errors are handled elsewhere
rg "removeResources" test/extended/authentication/ -B 2 -A 1Repository: openshift/origin
Length of output: 1967
🏁 Script executed:
# Verify the guideline interpretation: Check Go security guidelines in coding_guidelines
# Look for the exact context usage guidance
rg "context.Context" test/extended/authentication/component_proxy.goRepository: openshift/origin
Length of output: 553
Add timeout context for cleanup operations and handle errors.
Line 33 assigns context.Background() which has no cancellation or deadline. When cleanup operations in AfterEach (line 63) reuse this context, a stalled API request can hang indefinitely. Line 63 also ignores the error return from removeResources.
In AfterEach, create a time-bounded cleanup context: cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second); defer cancel(). Use cleanupCtx instead of the root ctx for removeResources. Check the error return or document explicitly why it is safe to discard.
The coding guidelines require context with cancellation/timeout for operations and never to ignore error returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy.go` around lines 32 - 34, Update
the test setup and cleanup flow around BeforeEach and AfterEach to create a
30-second timeout context with cancellation for cleanup, pass that cleanup
context to removeResources instead of the root ctx, and handle the returned
error explicitly rather than ignoring it.
Sources: Path instructions, Learnings
| g.AfterEach(func() { | ||
| _ = removeResources(ctx, cleanups...) | ||
|
|
||
| g.By("Waiting for operators to be stable after test") | ||
| err := operator.WaitForOperatorsToSettle(ctx, oc.AdminConfigClient(), 10) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report cleanup failures.
Line 63 discards the error from removeResources. If state restoration or resource deletion fails, later serial specs can run against leaked authentication configuration. Capture and assert this error.
As per path instructions, Go code must never ignore error returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy.go` around lines 62 - 67, The
removeResources function call in the AfterEach block discards its error return
value using the blank identifier, which violates Go error handling practices and
could hide cleanup failures. Capture the error return from removeResources
instead of discarding it, then assert that the error did not occur using the
same expectation pattern applied to the WaitForOperatorsToSettle error below it,
ensuring cleanup failures are properly reported before the test completes.
Source: Path instructions
|
Scheduling required tests: |
|
@ehearne-redhat: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
The ip_whitelist annotation doesn't work on cloud clusters where the proxy resolves the route hostname to the external LB IP — the ingress router sees the LB's source IP instead of the proxy pod IP. Replace the route restriction with a squid access log check that verifies the operator's pod IP appears in a successful CONNECT tunnel entry. Use a custom logformat directive for precise, structured log parsing (method, URL, source IP, status, HTTP code). Remove restrictKeycloakRouteToProxy since it's no longer used.
this change migrate tests over + add helper for to/from traffic check
1a6a523 to
b742e6b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ehearne-redhat
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@ehearne-redhat: This pull request references CNTRLPLANE-3851 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target either version "5.0." or "openshift-5.0.", but it targets "openshift-5.1" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
test/extended/authentication/component_proxy_helpers.go (1)
201-201: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
new(int32(1))does not compile.The
newbuiltin accepts a type, not a value. Useptr.To(int32(1)).🐛 Proposed fix
- Replicas: new(int32(1)), + Replicas: ptr.To(int32(1)),Add the import:
"k8s.io/utils/ptr"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_helpers.go` at line 201, Fix the Replicas assignment in the affected component proxy helper by replacing the invalid new(int32(1)) expression with ptr.To(int32(1)), and add the k8s.io/utils/ptr import.test/extended/authentication/component_proxy.go (2)
81-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport cleanup failures and bound the cleanup context.
Line 82 discards the error from
removeResources. A failed restore then stays invisible, and later serial specs run against leaked authentication configuration. The sharedctxfrom line 45 has no deadline, so a stalled API call in cleanup can hang the suite.Assert the error and use a time-bounded cleanup context.
🐛 Proposed fix
g.AfterEach(func() { - _ = removeResources(ctx, cleanups...) + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Minute) + defer cancel() + err := removeResources(cleanupCtx, cleanups...) + o.Expect(err).NotTo(o.HaveOccurred(), "cleanup should succeed") g.By("Waiting for operators to be stable after test") - err := operator.WaitForOperatorsToSettle(ctx, oc.AdminConfigClient(), 10) + err = operator.WaitForOperatorsToSettle(cleanupCtx, oc.AdminConfigClient(), 10) o.Expect(err).NotTo(o.HaveOccurred()) })As per path instructions: "Never ignore error returns" and "context.Context for cancellation and timeouts".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy.go` around lines 81 - 87, Update the g.AfterEach cleanup around removeResources to create and use a time-bounded context, ensuring stalled cleanup calls are cancelled; stop discarding removeResources errors and assert/report them before waiting for operators to settle. Preserve the existing operator stabilization check and cleanup ordering.Source: Path instructions
49-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAppend cleanup functions only after you assert the error.
saveAndRestoreAuthStateanddeploySquidProxyreturn a nilremovalFuncon some failure paths. Each call appends the returned function tocleanupsbefore the assertion. When the helper fails, Gomega aborts the spec,AfterEachruns, andremoveResourcesinvokes a nil function. The resulting nil-call panic replaces the real assertion failure. Move eacho.Expect(err)above the correspondingappend, or skip nil entries.#!/bin/bash # Check whether removeResources guards against nil removalFunc entries. rg -n 'func removeResources' -A 20 test/extended/authentication rg -n 'type removalFunc' -C2 test/extended/authentication🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy.go` around lines 49 - 63, In the setup flow, update saveAndRestoreAuthState and deploySquidProxy so each returned cleanup is appended to cleanups only after its corresponding o.Expect(err).NotTo(o.HaveOccurred()) assertion succeeds; alternatively, ensure removeResources safely skips nil removalFunc entries. Preserve cleanup registration for successful helper calls and prevent failed calls from causing a nil-function panic.
🧹 Nitpick comments (6)
test/extended/authentication/component_proxy.go (4)
329-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not reuse the operator-managed ConfigMap name for the user-supplied trustedCA.
componentProxyCAConfigMapNameis the name that the operator creates inopenshift-authenticationfor the synchronized bundle.verifyTrustedCAConfigMapSyncedchecks that exact name in that namespace. Creating a source ConfigMap with the same name inopenshift-configmakes the two roles indistinguishable in the test, and the cleanup on line 342 deletes only theopenshift-configcopy. Use a distinct source name, astestPartialFullEnvVarsdoes withe2e-proxy-trusted-ca.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy.go` around lines 329 - 337, Use a distinct user-supplied trustedCA ConfigMap name in the ConfigMap definition near caConfigMap instead of componentProxyCAConfigMapName, matching the separate source-name pattern used by testPartialFullEnvVars (for example, e2e-proxy-trusted-ca). Keep componentProxyCAConfigMapName reserved for the operator-managed synchronized bundle.
430-436: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant string conversion.
Output()already returns astring.string(output)is an unnecessary conversion, andunconvert-style linters inmake verifycan report it.♻️ Proposed change
- o.Expect(err).NotTo(o.HaveOccurred(), "squid reconfigure failed: %s", string(output)) + o.Expect(err).NotTo(o.HaveOccurred(), "squid reconfigure failed: %s", output)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy.go` around lines 430 - 436, In the squid reconfigure assertion, update the error message in the `oc.AsAdmin().Run("exec").Args(...).Output()` flow to use `output` directly instead of converting it with `string(output)`. Preserve the existing command execution and error assertion behavior.
257-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the expected
NO_PROXYvalue fromnoProxyHost.Line 258 repeats the literal
noproxy.example.com. The value on line 241 and the expectation can drift apart. Compose the expected string from the variable.♻️ Proposed change
- err = verifyOAuthServerDeploymentProxyConfig(ctx, oc, httpProxyURL, httpsProxyURL, ".cluster.local,.svc,127.0.0.1,localhost,noproxy.example.com", true) + err = verifyOAuthServerDeploymentProxyConfig(ctx, oc, httpProxyURL, httpsProxyURL, + ".cluster.local,.svc,127.0.0.1,localhost,"+noProxyHost, true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy.go` around lines 257 - 259, Update the expected NO_PROXY argument in the test around verifyOAuthServerDeploymentProxyConfig to compose its custom host entry from the existing noProxyHost variable instead of repeating the noproxy.example.com literal, while preserving the surrounding default entries and verification behavior.
318-323: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTwo negative traffic assertions use a fixed two-minute sleep. Both sites wait a fixed period and then assert that the Squid access log contains no Keycloak host. The shared root cause is the fixed delay used as a settle window; a bounded poll gives the same guarantee with less fixed runtime in this serial suite.
test/extended/authentication/component_proxy.go#L318-L323: replacetime.Sleep(2 * time.Minute)with a bounded poll of the access log after you confirm the direct login succeeded.test/extended/authentication/component_proxy.go#L493-L498: apply the same bounded poll for thenoProxycase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy.go` around lines 318 - 323, The negative Squid traffic checks use an unnecessary fixed two-minute delay. In test/extended/authentication/component_proxy.go#L318-L323 and `#L493-L498`, replace each time.Sleep-based settle window with the existing bounded polling mechanism that repeatedly reads the access log after direct login succeeds, then preserve the assertions that the logs do not contain keycloakHost.test/extended/authentication/component_proxy_helpers.go (2)
576-588: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail early if
ca-bundle.crtis absent.If
default-ingress-certdoes not containca-bundle.crt, this helper creates a ConfigMap with an emptyca.crt. The IdP then fails TLS verification, and the failure appears much later as a login timeout. Check the key and return an error.♻️ Proposed change
+ caBundle, ok := ca.Data["ca-bundle.crt"] + if !ok || len(caBundle) == 0 { + return nil, fmt.Errorf("openshift-config-managed/default-ingress-cert has no ca-bundle.crt") + } + _, err = kubeClient.CoreV1().ConfigMaps("openshift-config").Create(ctx, &corev1.ConfigMap{ @@ Data: map[string]string{ - "ca.crt": ca.Data["ca-bundle.crt"], + "ca.crt": caBundle, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_helpers.go` around lines 576 - 588, Update the helper around the retrieved ConfigMap and ca.Data["ca-bundle.crt"] to validate that the ca-bundle.crt key exists before creating the new ConfigMap. If absent, return a descriptive error immediately; otherwise preserve the existing ca.crt assignment and creation flow.
313-328: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead logs from all Squid pods, not only the first list entry.
pods.Items[0]can select a terminating or newly created pod. The access-log assertions then miss entries and the polling helpers time out. Filter for a running pod, or concatenate the logs of all matching pods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/authentication/component_proxy_helpers.go` around lines 313 - 328, Update the log retrieval flow using the listed Squid pods so it does not rely on pods.Items[0]. Select an appropriate running pod or aggregate logs from every matching pod, while preserving the existing container and since-time options and returning the combined access logs to the polling helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 664-666: Update matchTrustedCAVolume to return the two observed
volume and mount-state booleans, then use those values in the GinkgoWriter
message instead of !expectTrustedCAVolume. Preserve the existing match result
and expected-value reporting.
- Around line 84-112: Update the cleanup function to collect restore errors from
both the authentication/cluster and oauth/cluster RetryOnConflict calls, while
continuing to attempt both restores. Preserve the existing logging and
operator-stabilization behavior, then return the combined failure using the
errors package instead of always returning nil.
- Around line 436-454: The client discovery loop should not select an arbitrary
client based on non-empty RedirectURIs. Update the logic around adminClientID,
passwdClientID, and setup.clientID to match the intended built-in Keycloak
client by its exact ClientID, or create and select a dedicated client, while
preserving the existing missing-client errors and secret/provider setup.
---
Duplicate comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Line 201: Fix the Replicas assignment in the affected component proxy helper
by replacing the invalid new(int32(1)) expression with ptr.To(int32(1)), and add
the k8s.io/utils/ptr import.
In `@test/extended/authentication/component_proxy.go`:
- Around line 81-87: Update the g.AfterEach cleanup around removeResources to
create and use a time-bounded context, ensuring stalled cleanup calls are
cancelled; stop discarding removeResources errors and assert/report them before
waiting for operators to settle. Preserve the existing operator stabilization
check and cleanup ordering.
- Around line 49-63: In the setup flow, update saveAndRestoreAuthState and
deploySquidProxy so each returned cleanup is appended to cleanups only after its
corresponding o.Expect(err).NotTo(o.HaveOccurred()) assertion succeeds;
alternatively, ensure removeResources safely skips nil removalFunc entries.
Preserve cleanup registration for successful helper calls and prevent failed
calls from causing a nil-function panic.
---
Nitpick comments:
In `@test/extended/authentication/component_proxy_helpers.go`:
- Around line 576-588: Update the helper around the retrieved ConfigMap and
ca.Data["ca-bundle.crt"] to validate that the ca-bundle.crt key exists before
creating the new ConfigMap. If absent, return a descriptive error immediately;
otherwise preserve the existing ca.crt assignment and creation flow.
- Around line 313-328: Update the log retrieval flow using the listed Squid pods
so it does not rely on pods.Items[0]. Select an appropriate running pod or
aggregate logs from every matching pod, while preserving the existing container
and since-time options and returning the combined access logs to the polling
helpers.
In `@test/extended/authentication/component_proxy.go`:
- Around line 329-337: Use a distinct user-supplied trustedCA ConfigMap name in
the ConfigMap definition near caConfigMap instead of
componentProxyCAConfigMapName, matching the separate source-name pattern used by
testPartialFullEnvVars (for example, e2e-proxy-trusted-ca). Keep
componentProxyCAConfigMapName reserved for the operator-managed synchronized
bundle.
- Around line 430-436: In the squid reconfigure assertion, update the error
message in the `oc.AsAdmin().Run("exec").Args(...).Output()` flow to use
`output` directly instead of converting it with `string(output)`. Preserve the
existing command execution and error assertion behavior.
- Around line 257-259: Update the expected NO_PROXY argument in the test around
verifyOAuthServerDeploymentProxyConfig to compose its custom host entry from the
existing noProxyHost variable instead of repeating the noproxy.example.com
literal, while preserving the surrounding default entries and verification
behavior.
- Around line 318-323: The negative Squid traffic checks use an unnecessary
fixed two-minute delay. In
test/extended/authentication/component_proxy.go#L318-L323 and `#L493-L498`,
replace each time.Sleep-based settle window with the existing bounded polling
mechanism that repeatedly reads the access log after direct login succeeds, then
preserve the assertions that the logs do not contain keycloakHost.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8fcdcb62-5ef7-4ae2-8b27-b271d8d252cb
📒 Files selected for processing (4)
test/extended/authentication/component_proxy.gotest/extended/authentication/component_proxy_helpers.gotest/extended/authentication/keycloak_client.gotest/extended/authentication/operator_status_helpers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended/authentication/operator_status_helpers.go
- test/extended/authentication/keycloak_client.go
| }); err != nil { | ||
| g.GinkgoWriter.Printf("cleanup: failed to restore Authentication CR: %v\n", err) | ||
| } | ||
|
|
||
| g.GinkgoWriter.Println("cleanup: restoring oauth/cluster") | ||
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| fresh, err := oauthClient.Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if reflect.DeepEqual(fresh.Spec, *originalOAuthSpec) { | ||
| return nil | ||
| } | ||
| changed = true | ||
| fresh.Spec = *originalOAuthSpec | ||
| _, err = oauthClient.Update(ctx, fresh, metav1.UpdateOptions{}) | ||
| return err | ||
| }); err != nil { | ||
| g.GinkgoWriter.Printf("cleanup: failed to restore oauth/cluster: %v\n", err) | ||
| } | ||
|
|
||
| if changed { | ||
| g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize") | ||
| if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil { | ||
| g.GinkgoWriter.Printf("cleanup: operator did not recover: %v\n", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return restore failures instead of only logging them.
The cleanup function always returns nil. If the restore of authentication/cluster or oauth/cluster fails, the caller cannot detect it. This suite is [Serial], so leaked proxy or IdP configuration affects later specs. Collect the failures and return them, while still attempting both restores.
♻️ Proposed change
return func(ctx context.Context) error {
var changed bool
+ var errs []error
g.GinkgoWriter.Println("cleanup: restoring authentication/cluster")
if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
@@
}); err != nil {
- g.GinkgoWriter.Printf("cleanup: failed to restore Authentication CR: %v\n", err)
+ errs = append(errs, fmt.Errorf("restoring authentication/cluster: %w", err))
}
@@
}); err != nil {
- g.GinkgoWriter.Printf("cleanup: failed to restore oauth/cluster: %v\n", err)
+ errs = append(errs, fmt.Errorf("restoring oauth/cluster: %w", err))
}
if changed {
g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize")
if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil {
- g.GinkgoWriter.Printf("cleanup: operator did not recover: %v\n", err)
+ errs = append(errs, fmt.Errorf("operator did not recover: %w", err))
}
}
- return nil
+ return errors.Join(errs...)
}, nilAdd the errors import.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }); err != nil { | |
| g.GinkgoWriter.Printf("cleanup: failed to restore Authentication CR: %v\n", err) | |
| } | |
| g.GinkgoWriter.Println("cleanup: restoring oauth/cluster") | |
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | |
| fresh, err := oauthClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| if reflect.DeepEqual(fresh.Spec, *originalOAuthSpec) { | |
| return nil | |
| } | |
| changed = true | |
| fresh.Spec = *originalOAuthSpec | |
| _, err = oauthClient.Update(ctx, fresh, metav1.UpdateOptions{}) | |
| return err | |
| }); err != nil { | |
| g.GinkgoWriter.Printf("cleanup: failed to restore oauth/cluster: %v\n", err) | |
| } | |
| if changed { | |
| g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize") | |
| if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil { | |
| g.GinkgoWriter.Printf("cleanup: operator did not recover: %v\n", err) | |
| } | |
| } | |
| return nil | |
| }, nil | |
| return func(ctx context.Context) error { | |
| var changed bool | |
| var errs []error | |
| g.GinkgoWriter.Println("cleanup: restoring authentication/cluster") | |
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | |
| fresh, err := authenticationClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| if reflect.DeepEqual(fresh.Spec, *originalAuthenticationSpec) { | |
| return nil | |
| } | |
| changed = true | |
| fresh.Spec = *originalAuthenticationSpec | |
| _, err = authenticationClient.Update(ctx, fresh, metav1.UpdateOptions{}) | |
| return err | |
| }); err != nil { | |
| errs = append(errs, fmt.Errorf("restoring authentication/cluster: %w", err)) | |
| } | |
| g.GinkgoWriter.Println("cleanup: restoring oauth/cluster") | |
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | |
| fresh, err := oauthClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| if reflect.DeepEqual(fresh.Spec, *originalOAuthSpec) { | |
| return nil | |
| } | |
| changed = true | |
| fresh.Spec = *originalOAuthSpec | |
| _, err = oauthClient.Update(ctx, fresh, metav1.UpdateOptions{}) | |
| return err | |
| }); err != nil { | |
| errs = append(errs, fmt.Errorf("restoring oauth/cluster: %w", err)) | |
| } | |
| if changed { | |
| g.GinkgoWriter.Println("cleanup: waiting for operator to stabilize") | |
| if err := waitForOperatorToPickUpChanges(ctx, oc, "authentication"); err != nil { | |
| errs = append(errs, fmt.Errorf("operator did not recover: %w", err)) | |
| } | |
| } | |
| return errors.Join(errs...) | |
| }, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy_helpers.go` around lines 84 -
112, Update the cleanup function to collect restore errors from both the
authentication/cluster and oauth/cluster RetryOnConflict calls, while continuing
to attempt both restores. Preserve the existing logging and
operator-stabilization behavior, then return the combined failure using the
errors package instead of always returning nil.
| var adminClientID, passwdClientID string | ||
| for _, c := range clientList { | ||
| if c.ClientID == "admin-cli" { | ||
| adminClientID = c.ID | ||
| } else if len(c.RedirectURIs) > 0 { | ||
| passwdClientID = c.ID | ||
| setup.clientID = c.ClientID | ||
| } | ||
| if len(passwdClientID) > 0 && len(adminClientID) > 0 { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if adminClientID == "" { | ||
| return nil, cleanups, fmt.Errorf("admin-cli client not found in keycloak") | ||
| } | ||
| if passwdClientID == "" { | ||
| return nil, cleanups, fmt.Errorf("password-grant client (with redirectUris) not found in keycloak") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Keycloak fixture and the client(s) it provisions.
rg -n 'func deployKeycloak\b' -A 60 test/extended/authentication
rg -n 'RedirectURIs|clientID|ClientID' test/extended/authentication --glob '*.go' -C2Repository: openshift/origin
Length of output: 14158
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- deployKeycloakForProxy context ---'
sed -n '380,565p' test/extended/authentication/component_proxy_helpers.go
printf '%s\n' '--- client setup and callers ---'
rg -n 'deployKeycloakForProxy|passwdClientID|password-grant|ConfigureClient|clientId|clientID' test/extended/authentication --glob '*.go' -C3
printf '%s\n' '--- Keycloak image and fixture configuration ---'
rg -n 'KEYCLOAK|keycloak|admin-cli|account-console|security-admin-console|redirectUris|clientId' test/extended/authentication --glob '*.{go,yaml,yml,json,sh}' -C2Repository: openshift/origin
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Keycloak deployment definition ---'
rg -n 'func createKeycloakDeployment\b' -A 100 test/extended/authentication/keycloak_helpers.go
printf '%s\n' '--- proxy caller and client mutation ---'
sed -n '50,90p' test/extended/authentication/component_proxy.go
printf '%s\n' '--- all client-related fixture writes ---'
rg -n 'CreateClient|POST.*clients|/clients|directAccessGrantsEnabled|RegenerateClientSecret' test/extended/authentication --glob '*.go' -C2Repository: openshift/origin
Length of output: 11543
🌐 Web query:
Keycloak 25 default master realm clients redirectUris account account-console security-admin-console client secret regenerate
💡 Result:
In Keycloak 25, the master realm includes several built-in clients such as security-admin-console and account-console [1]. These clients are pre-configured by Keycloak [2]. Default Redirect URIs For the security-admin-console client, the default redirect URI is typically set to /admin/master/console/* [3]. Note that in Keycloak 25, there are specific changes regarding the handling of the ${authAdminUrl} and hostname configuration [3]. It is recommended to use the hostname-admin option to manage hostnames rather than relying on wildcard redirects, as improper configuration may cause issues with alternative hostnames [3]. Regenerating Client Secrets You can regenerate a client secret for any client (including default ones, if applicable) using the Keycloak Admin REST API [4][5]. To regenerate a secret via the REST API: 1. Identify the internal ID of the client (this is a UUID, not the client ID like security-admin-console) [4][5]. You can retrieve this by listing clients in the realm [6]. 2. Perform a POST request to the following endpoint: {auth-server-url}/admin/realms/{realm}/clients/{client-internal-id}/client-secret [5] The response will contain the new client secret [5]. Note that this operation requires appropriate administrative permissions [4]. Tools like kcadm.sh can also be used to facilitate this by calling the same underlying API endpoints [6]. If using Ansible, the community.general.keycloak_clientsecret_regenerate module is available to automate this process [4].
Citations:
- 1: https://www.keycloak.org/docs-api/25.0.6/javadocs/org/keycloak/models/Constants.html
- 2: Idea: Don't use widcards for valid redirect URIs of admin console and and account console keycloak/keycloak#28922
- 3: https://github.com/keycloak/keycloak/blob/main/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc
- 4: https://docs.ansible.com/ansible/latest/collections/community/general/keycloak_clientsecret_regenerate_module.html
- 5: https://forum.keycloak.org/t/how-to-create-client-secret-via-api/20198
- 6: https://github.com/keycloak/keycloak/blob/1ecbb5d0/docs/documentation/server_admin/topics/admin-cli.adoc
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Keycloak container command and environment ---'
sed -n '100,180p' test/extended/authentication/keycloak_helpers.go
sed -n '261,330p' test/extended/authentication/keycloak_helpers.go
printf '%s\n' '--- Keycloak 25 default client definitions ---'
curl -fsSL https://raw.githubusercontent.com/keycloak/keycloak/25.0.6/services/src/main/java/org/keycloak/services/resources/KeycloakApplication.java |
rg -n 'account|admin-cli|security-admin-console|broker|client'
curl -fsSL https://raw.githubusercontent.com/keycloak/keycloak/25.0.6/services/src/main/java/org/keycloak/models/utils/RepresentationToModel.java |
rg -n 'account|admin-cli|security-admin-console|redirectUris|clientId'Repository: openshift/origin
Length of output: 4462
🌐 Web query:
site:github.com/keycloak/keycloak "security-admin-console" "redirectUris" "account-console"
💡 Result:
In Keycloak, the security-admin-console and account-console are built-in clients used by the administration and user account consoles, respectively [1][2]. The security-admin-console client uses the ${authAdminUrl} variable for its default redirect URIs (e.g., /admin/master/console/*) [3][4]. This variable resolves dynamically based on Keycloak's hostname configuration [3][4]. Key Considerations: 1. Configuration over Modification: It is generally discouraged to manually modify the redirectUris of these built-in clients [3]. Instead, you should configure the hostname-admin option in your Keycloak server configuration to correctly define the URL for the administration console [5][4]. Manually adding URIs can lead to unexpected behavior or security misconfigurations [3]. 2. Hostname v2 Changes: With the introduction of "hostname v2" (Keycloak 25+), the authAdminUrl defaults to the frontend URL if hostname-admin is not explicitly set [4]. This change means that if you are using custom hostnames or reverse proxies, you should prioritize setting the hostname-admin option rather than manually updating redirect URIs to include alternative hostnames [4]. 3. Account Console: The account-console client is also a built-in client [2]. Similar to the admin console, its redirect URIs are often relative paths [2]. If you modify realm identifiers, these relative URLs may not update automatically, requiring manual verification [2]. In summary, for setups involving reverse proxies or custom hostnames, rely on the hostname-admin server configuration option to handle redirection, as modifying the security-admin-console client directly is not the recommended approach and may break deep linking or cause security issues [5][3][4].
Citations:
- 1: Logged in user has to login again on account console page of realm keycloak/keycloak#42823
- 2: Changing realm id will not update relative URLs in
account-consoleclient keycloak/keycloak#17010 - 3: Admin Console Spins with hostname:v2 using security-admin-console Redirect URIs keycloak/keycloak#31640
- 4: https://github.com/keycloak/keycloak/blob/main/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc
- 5: Exposing the Admin Console Using a Different URL keycloak/keycloak#12186
🏁 Script executed:
#!/bin/bash
set -e
tmp=$(mktemp)
curl -fsSL 'https://api.github.com/repos/keycloak/keycloak/git/trees/25.0.6?recursive=1' > "$tmp"
jq -r '.tree[] | select(.path | test("(?i)(realm|client|import|bootstrap)")) | .path' "$tmp" |
rg '(^|/)(import|bootstrap|realm|client)|json$|yaml$' |
head -80
rm -f "$tmp"Repository: openshift/origin
Length of output: 205
Select the intended Keycloak client by exact ClientID.
deployKeycloak does not create a dedicated client. This loop selects the first built-in client with non-empty RedirectURIs, then regenerates its secret and uses its ID for the OIDC provider. Match the intended built-in client explicitly or create a dedicated client instead of using RedirectURIs as the selector.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy_helpers.go` around lines 436 -
454, The client discovery loop should not select an arbitrary client based on
non-empty RedirectURIs. Update the logic around adminClientID, passwdClientID,
and setup.clientID to match the intended built-in Keycloak client by its exact
ClientID, or create and select a dedicated client, while preserving the existing
missing-client errors and secret/provider setup.
| if !matchTrustedCAVolume(deployment, expectTrustedCAVolume) { | ||
| g.GinkgoWriter.Printf("trustedCA volume/mount present=%v (want present=%v)\n", !expectTrustedCAVolume, expectTrustedCAVolume) | ||
| return false, nil |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The log message prints the expectation, not the observed state.
!expectTrustedCAVolume is the inverse of the wanted value, not the actual value. When expectTrustedCAVolume is true, the message always reports present=false, even if only the mount is missing. Return the observed volume and mount state and log it.
🐛 Proposed fix
- if !matchTrustedCAVolume(deployment, expectTrustedCAVolume) {
- g.GinkgoWriter.Printf("trustedCA volume/mount present=%v (want present=%v)\n", !expectTrustedCAVolume, expectTrustedCAVolume)
+ foundVolume, foundMount := findTrustedCAVolume(deployment)
+ if (foundVolume && foundMount) != expectTrustedCAVolume {
+ g.GinkgoWriter.Printf("trustedCA volume present=%v mount present=%v (want present=%v)\n", foundVolume, foundMount, expectTrustedCAVolume)
return false, nil
}Change matchTrustedCAVolume to return the two observed booleans.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/authentication/component_proxy_helpers.go` around lines 664 -
666, Update matchTrustedCAVolume to return the two observed volume and
mount-state booleans, then use those values in the GinkgoWriter message instead
of !expectTrustedCAVolume. Preserve the existing match result and expected-value
reporting.
|
Scheduling required tests: |
| "directAccessGrantsEnabled": true, | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
There was a problem hiding this comment.
Since this is not needed in all the tests, it would be better to keep it in the relevant tests, even though it's mild code repetition.
| g.It("should fall back on spec.proxy removal", func() { | ||
| testFallbackOnProxyRemoval(ctx, oc, kcSetup, httpProxyURL, proxyNamespace) | ||
| }) | ||
| g.It("should set partial and full env vars when configured", func() { |
There was a problem hiding this comment.
We should improve test names TBH. When I read this without knowing the test really, I have no idea what the test is testing. I have to improve this in my PR as well, I think.
You can check full names with
$ ./openshift-tests list tests | grep -i componentproxy| challengehandlers.NewBasicChallengeHandler(oauthServerURL, "", nil, io.Discard, nil, username, password), | ||
| ) | ||
| if err != nil { | ||
| t.Logf("failed to create challenge handler: %v", err) |
There was a problem hiding this comment.
I think that we usually use g.GinkgoWriter.Printf in these tests, perhaps we should be consistent with that?
| o.Expect(err).NotTo(o.HaveOccurred(), "should be able to delete user %q", username) | ||
| } | ||
|
|
||
| func testPartialFullEnvVars(ctx context.Context, oc *exutil.CLI, caCertPEM []byte, httpProxyURL, httpsProxyURL string) { |
There was a problem hiding this comment.
This is gonna take a long time to just check the env vars because of all the re-deployments... We could merge this into some other test, which would be more efficient, but also more ugly. Just leaving as a note for now...
| var oauthPodIPs []string | ||
| for _, p := range oauthPods.Items { | ||
| oauthPodIPs = append(oauthPodIPs, p.Status.PodIP) | ||
| } |
There was a problem hiding this comment.
This block could be a tiny helper. It's used again in another test as well.
|
|
||
| func testHotReloadCAFileChange(ctx context.Context, oc *exutil.CLI, caCertPEM []byte, kcSetup *keycloakProxySetup, httpsProxyURL, proxyNamespace string) { | ||
| kcUser, kcPass, kcGroup := createKeycloakUserPasswordGroup(kcSetup) | ||
| g.By("Creating config map with trustedCA") |
There was a problem hiding this comment.
Could we pls prepend a newline before each By?
| err = waitForOperatorToPickUpChanges(ctx, oc, "authentication") | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| logCutOff := time.Now() |
There was a problem hiding this comment.
Again, actually just remove the proxy namespace already.
| if err := kubeClient.CoreV1().ConfigMaps("openshift-config").Delete(ctx, componentProxyCAConfigMapName, metav1.DeleteOptions{}); err != nil { | ||
| g.GinkgoWriter.Printf("failed to clean up ConfigMap %s: %v\n", componentProxyCAConfigMapName, err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Also I am doing the same thing in my trustedCA test, so we could refactor to use a helper. You can pick the config map name. You use an existing constant that is not related to this configmap, which is a bit confusing IMO.
|
|
||
| g.By("Verifying trustedCA ConfigMap is synced to openshift-authentication namespace") | ||
| err = verifyTrustedCAConfigMapSynced(ctx, oc) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
Shouldn't this call be way more up? I mean, at this point you can be sure it's sync'd, otherwise the login flow would not work, I think.
| deleteOIDCUserAndIdentities(ctx, oc, kcUser) | ||
|
|
||
| g.By("Verifying OIDC login works after CA rotation") | ||
| assertOIDCLogin(ctx, oc, kcUser, kcPass, kcGroup) |
There was a problem hiding this comment.
I am wondering whether this can fail in case trustedCA is somehow not synchronized in time. But I am also not sure what to do about this. I guess you would need to keep checking the actual file in the pod to contain the right trusted CA...
3 participants
Continues work from openshift/cluster-authentication-operator#950 . Kept in separate file for now. Plan is to add tests to
test/extended/authentication/component_proxy_oauth.gointo #31446 when consensus reached on test status.Summary by CodeRabbit